Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added condition when downloading record for notification with stream_status finished #102

Merged
merged 5 commits into from
Mar 11, 2024

Conversation

vitorsemeano
Copy link
Contributor

@vitorsemeano vitorsemeano commented Mar 4, 2024

Closes JurajNyiri/HomeAssistant-Tapo-Control#523

During the download of a record, the camera is sending a json response, notifying that the download finished.

Since the code is not ready for this notification, it never assumed a complete record, entering in a loop between retrying and downloading the same record.

This code prevents the situation, expecting to find the json response, and if such situation occurs, assumes that the download is finished.

For reference, the response to find is the following:

b'{"type":"notification", "params":{"event_type":"stream_status", "status":"finished"}}'

Also added missing dependency rtp for tox.

@vitorsemeano vitorsemeano changed the title Added condition when downloading record for notification with stream_status finished Draft: Added condition when downloading record for notification with stream_status finished Mar 6, 2024
@vitorsemeano vitorsemeano marked this pull request as draft March 6, 2024 17:59
@vitorsemeano vitorsemeano changed the title Draft: Added condition when downloading record for notification with stream_status finished Added condition when downloading record for notification with stream_status finished Mar 6, 2024
added missing imports in downloader.py
@vitorsemeano vitorsemeano marked this pull request as ready for review March 7, 2024 00:19
@JurajNyiri JurajNyiri self-requested a review March 8, 2024 12:31
Copy link
Owner

@JurajNyiri JurajNyiri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your PR!
Code changes look good, tested with my camera working. Just minor update needed.
Let me know if you can do it or if not I can do it after merging your PR.

Comment on lines 9 to 11
import logging

logger = logging.getLogger(__name__)
Copy link
Owner

@JurajNyiri JurajNyiri Mar 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logger is passed from Home Assistant to init.py as printDebugInformation parameter.
I believe we should use the same approach here, use tapo object passed to Downloader class and use its function debugLog.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can delete this and use self.tapo.debugLog() call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleted and replaced logger.warning with debugLog

Comment on lines 288 to 293
# print(plaintext)

# # Update our own sequence numbers to avoid collisions
# if (seq is not None) and (seq > self._seq_counter):
# self._seq_counter = seq + 1

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove comments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed this block of comments and also the print comments in this file for clarity.

downloading = False
break
except JSONDecodeError:
logger.warning("Unable to parse JSON sent from device")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use self.tapo.debugLog() call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@vitorsemeano
Copy link
Contributor Author

Thank you for your PR! Code changes look good, tested with my camera working. Just minor update needed. Let me know if you can do it or if not I can do it after merging your PR.

Sure, i will do the changes now and push shortly.

@vitorsemeano
Copy link
Contributor Author

@JurajNyiri all done, if you need something else, just ask.

@JurajNyiri JurajNyiri merged commit dcbc991 into JurajNyiri:main Mar 11, 2024
@JurajNyiri
Copy link
Owner

Thank you!

JurajNyiri added a commit that referenced this pull request Mar 11, 2024
@JurajNyiri
Copy link
Owner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fails to syncronize some recordings, resulting in endless retries
2 participants